feat(linter): adds jsx-a11y/no-static-element-interactions rule#14922
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
* includes codegen for npm `aria-query` & `axobject-query` data required for this and other jsx-a11y rule implementations
92df135 to
fba3410
Compare
camchenry
left a comment
There was a problem hiding this comment.
I think I'm in support of having our own version of aria-query in theory, but it's a big undertaking. I left some small suggestions in the PR, but bigger suggestions I have:
- Is there another simpler rule that requires a subset of this information? It would be simpler if we didn't need to migrate all of the data all at once.
- Maybe we could split the
oxc_aria_querycrate into its own separate PR? That would make this a bit simpler to review. It's also lacking tests, documentation and such that would be suitable for a published crate.
| (r#"<aside aria-label onClick={() => {}} />;"#, None), | ||
| ]; | ||
|
|
||
| let fail = vec![ |
There was a problem hiding this comment.
Would you mind porting over the tests from the original rule? https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/a7d1a12a6198d546c4a06477b385b4fde03b762e/__tests__/src/rules/no-static-element-interactions-test.js#L4
There are a lot of tests currently missing.
| pub fn first_valid_role(jsx_opening_el: &JSXOpeningElement) -> Option<String> { | ||
| has_jsx_prop(jsx_opening_el, "role") | ||
| .and_then(get_string_literal_prop_value) | ||
| .map(|value| value.to_ascii_lowercase()) | ||
| .and_then(|lower| { | ||
| lower | ||
| .split_whitespace() | ||
| .find(|name| ABSTRACT_ROLES.contains(name) | ||
| || INTERACTIVE_ROLES.contains(name) | ||
| || NONINTERACTIVE_ROLES.contains(name)) | ||
| .map(|role| role.to_string()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Instead of simply returning a string (which loses the info about what role was contained here), how about parsing out the role?
This is a slightly bigger refactoring, but it would save us from needing to re-check what list of roles contains this value.
Something like this would be a good start at least:
enum AriaRoleType {
Interactive(String)
Noninteractive(String)
Abstract(String)
}
Then, instead of having is_interactive_role functions we could do:
impl AriaRoleType {
fn is_interactive(&self) -> bool { }
fn is_noninteractive(&self) -> bool { }
// ...
}And at that point, it shouldn't contain any linter-specific code, so we could move it into oxc_aria_query as well.
| if is_interactive_element(element_type.as_ref(), jsx_opening_el) | ||
| || is_interactive_role(jsx_opening_el) | ||
| || is_noninteractive_element(element_type.as_ref(), jsx_opening_el) | ||
| || is_noninteractive_role(element_type.as_ref(), jsx_opening_el) | ||
| || is_abstract_role(element_type.as_ref(), jsx_opening_el) | ||
| { | ||
| // This rule has no opinion about abstract roles. | ||
| return; | ||
| } |
There was a problem hiding this comment.
This comment doesn't seem to match the actual behavior, perhaps it should be more like this?
| if is_interactive_element(element_type.as_ref(), jsx_opening_el) | |
| || is_interactive_role(jsx_opening_el) | |
| || is_noninteractive_element(element_type.as_ref(), jsx_opening_el) | |
| || is_noninteractive_role(element_type.as_ref(), jsx_opening_el) | |
| || is_abstract_role(element_type.as_ref(), jsx_opening_el) | |
| { | |
| // This rule has no opinion about abstract roles. | |
| return; | |
| } | |
| if is_abstract_role(element_type.as_ref(), jsx_opening_el) | |
| { | |
| // This rule has no opinion about abstract roles. | |
| return; | |
| } | |
| if is_interactive_element(element_type.as_ref(), jsx_opening_el) | |
| && is_interactive_role(jsx_opening_el) | |
| && is_noninteractive_element(element_type.as_ref(), jsx_opening_el) | |
| && is_noninteractive_role(element_type.as_ref(), jsx_opening_el) { | |
| // actual rule logic here ... | |
| } |
|
|
||
| pub static INTERACTIVE_ELEMENT_SCHEMA: &[ElementSchema] = &[ | ||
| ElementSchema { | ||
| name: "a", |
There was a problem hiding this comment.
Storing these tag names multiple times bloats the binary size. We should store these as static values and reference them instead.
Example:
static TAG_A: &'static str = "a";
static TAG_AREA: &'static str = "area";
ElementSchema {
name: TAG_AREA,
attributes: &[]
}The same holds true for all property/attribute names too. If we could replace each of those with a reference to a static string, that would be a nice improvement.
There was a problem hiding this comment.
Is there any reason we need to commit these files? We don't use them except for generating the Rust code which should be equivalent, so these are just an intermediate artifact, right?
| await fs.writeFile( | ||
| "abstractRoles.json", | ||
| JSON.stringify(abstractRoles, null, 2) | ||
| ); | ||
|
|
||
| await fs.writeFile( | ||
| "interactiveRoles.json", | ||
| JSON.stringify(interactiveRoles, null, 2) | ||
| ); | ||
|
|
||
| await fs.writeFile( | ||
| "noninteractiveRoles.json", | ||
| JSON.stringify(nonInteractiveRoles, null, 2) | ||
| ); |
There was a problem hiding this comment.
These could be run in parallel with Promise.all, no? Unless there is a particular reason we need to run these all synchronously.
| ElementSchema { | ||
| name: "input", | ||
| attributes: &[ | ||
| Attr { | ||
| name: "type", | ||
| value: Some("button"), | ||
| }, | ||
| ], | ||
| }, | ||
| ElementSchema { | ||
| name: "input", | ||
| attributes: &[ | ||
| Attr { | ||
| name: "type", | ||
| value: Some("image"), | ||
| }, | ||
| ], | ||
| }, | ||
| ElementSchema { | ||
| name: "input", | ||
| attributes: &[ | ||
| Attr { | ||
| name: "type", | ||
| value: Some("reset"), | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
I know this is more or less a direct port of aria-query, but it seems like these values are stored suboptimally. If we're only interested in input element schemas, we have to iterate over the entire array to find them. It seems like it would be better to store separate arrays for each element name, and iterate over those arrays instead. That would also allow us to easily check if a given tag is in the schemas list too.
Something more like this:
"input" => &[
ElementSchema {
attributes: &[/* ... */]
},
ElementSchema {
attributes: &[/* ... */]
},
],
|
This has been implemented via #17538, see no_static_element_interactions.rs. Is there a concern with regards to the accuracy/data used for this rule that warrants pulling in the additional crate? |
|
This PR includes some roles that are not included in the #17538 such as |
What's New
jsx-a11y/no-static-element-interactions- ☂️ eslint-plugin-jsx-a11y #1141tasks/a11y_dataoxc/oxc_aria_queryData Needs
This rule relies on data from
aria-queryandaxobject-querynpm packages. These data are not available in any well-maintained Rust crates from my search.There's a comment in
oxc_linter/src/utils/react.rsthat suggests aoxc-project/aria-queryrepo is a W.I.P. but that repo is labeled as "under construction" and now appears to be archived.I found no source for
axobject-querydata in Rust.I followed the lead of
tasks/compat_dataandcrates/oxc_compatwhich is similarly converting data from an npm package into Rust code.I opted to grab the minimum set of data required for this rule, but even this set should be enough to help with other unimplemented
jsx-a11yplugin rules, likeinteractive-supports-focus, that rely on the same data.Known Differences from ESLint Implementation
I found two main issues with the ESLint implementation of this rule.
allowExpressionValuesrule were ternaries where both sides were string literals. I looked at the GH issue that led to the no-op code being added and I believe the no-op code was added due to some miscommunication. I suspect the issue reporter was using a version of thejsx-a11yplugin that didn't have theallowExpressionValuesoption at all. A year and a half later the ticket was picked up and that option would've existed onmainand I believe the contributor assumed that the user was on the latest code so they added some new code to handle that case which the latest code already permitted. Unsurprisingly their new test case passed as the "violation" had already been permitted by the latest code.roleattributes. Sometimes they use the entireroleattribute value to classify roles and other times they separate the string on whitespace and use the first valid role. This difference in behavior didn't seem intentional so I've made this more consistent by creating afirst_valid_role()function inreact.rsthat uses the latter strategy.